Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix migration rule #1011

Merged
merged 3 commits into from
Mar 20, 2017
Merged

Fix migration rule #1011

merged 3 commits into from
Mar 20, 2017

Conversation

efernandez
Copy link
Contributor

@efernandez efernandez commented Mar 2, 2017 •
edited
Loading

This PR is on top of #1010 and fixes the test provided there.

This also replaces #996

@mikepurvis @dirk-thomas

FYI @afakihcpr @servos

This was referenced Mar 2, 2017
Closed
Merged
@efernandez efernandez force-pushed the fix_migration_rule branch from 92f45ee to 4a52cca Compare March 6, 2017 16:42
@efernandez efernandez mentioned this pull request Mar 6, 2017
Merged
@efernandez efernandez force-pushed the fix_migration_rule branch from 4a52cca to 80db563 Compare March 7, 2017 23:01
@efernandez
Copy link
Contributor Author

@dirk-thomas FYI I've locally seen this unrelated test (https://github.com/ros/ros_comm/blob/lunar-devel/test/test_rosbag/test/test_bag.py#L328) failing intermittently:

$ make run_tests_test_rosbag_nosetests_test.test_bag.py
-- run_tests.py: execute commands
  /usr/bin/cmake -E make_directory /home/eperdomo/dev/ws/clearpath_ws/build/test_rosbag/test_results/test_rosbag
  /usr/bin/nosetests-2.7 -P --process-timeout=60 /home/eperdomo/dev/ws/clearpath_ws/src/ros_comm/test/test_rosbag/test/test_bag.py --with-xunit --xunit-file=/home/eperdomo/dev/ws/clearpath_ws/build/test_rosbag/test_results/test_rosbag/nosetests-test.test_bag.py.xml
 /tmp/test_rosbag_filter__2.bag                                                                                                               100%              6.2 KB 00:00    
 /tmp/tmpsCAnH2/test_rosbag_filter__2.bag                                                                                                     100%              7.5 KB 00:00    
 /tmp/tmpsCAnH2/test_rosbag_filter__3.bag                                                                                                     100%              7.5 KB 00:00    
...
======================================================================
FAIL: test_get_compression_info (test_bag.TestRosbag)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/eperdomo/dev/ws/clearpath_ws/src/ros_comm/test/test_rosbag/test/test_bag.py", line 328, in test_get_compression_info
    self.assertGreater(info.compressed, 900)
AssertionError: 900 not greater than 900

----------------------------------------------------------------------
Ran 18 tests in 4.223s

FAILED (failures=1)
-- run_tests.py: verify result "/home/eperdomo/dev/ws/clearpath_ws/build/test_rosbag/test_results/test_rosbag/nosetests-test.test_bag.py.xml"
Built target run_tests_test_rosbag_nosetests_test.test_bag.py
$ make run_tests_test_rosbag_nosetests_test.test_bag.py
-- run_tests.py: execute commands
  /usr/bin/cmake -E make_directory /home/eperdomo/dev/ws/clearpath_ws/build/test_rosbag/test_results/test_rosbag
  /usr/bin/nosetests-2.7 -P --process-timeout=60 /home/eperdomo/dev/ws/clearpath_ws/src/ros_comm/test/test_rosbag/test/test_bag.py --with-xunit --xunit-file=/home/eperdomo/dev/ws/clearpath_ws/build/test_rosbag/test_results/test_rosbag/nosetests-test.test_bag.py.xml
 /tmp/test_rosbag_filter__2.bag                                                                                                               100%              6.2 KB 00:00    
 /tmp/tmpGOevHQ/test_rosbag_filter__2.bag                                                                                                     100%              7.5 KB 00:00    
 /tmp/tmpGOevHQ/test_rosbag_filter__3.bag                                                                                                     100%              7.5 KB 00:00    
...
----------------------------------------------------------------------
Ran 18 tests in 4.221s

OK
-- run_tests.py: verify result "/home/eperdomo/dev/ws/clearpath_ws/build/test_rosbag/test_results/test_rosbag/nosetests-test.test_bag.py.xml"
Built target run_tests_test_rosbag_nosetests_test.test_bag.py

These are two consecutive runs. I've seen values as low as 889. Let me know if you want me to reduce the lower bound to 888.

Either way, this and the others tests pass for this PR.

@efernandez efernandez force-pushed the fix_migration_rule branch from 80db563 to 75e11de Compare March 9, 2017 00:08
Enrique Fernandez added 3 commits March 15, 2017 17:20
Add message generation 4 to test_rosbag
8cbbe6b 
This allows to check for the following case:

Consider the following scenario for a message G with the following
versions:

G0 -- rule.bmr --> G1 -- auto-generated rule --> G3
where G3 is the current msg, and G2 is the bag message that needs
migration.

We need to check for a migration rule like this first:

G2 --> G3
instead of the backwards ones that were generated before this bug fix:

G2 --> G1

Note that the lazy check to mark the rules as valid in the
make_update_rule (see
https://github.com/clearpathrobotics/ros_comm/blob/kinetic-devel-cpr/tools/rosbag/src/rosbag/migration.py#L1123)
method makes that rule pass initially, but later it fails because
there's no rule for the sub fields.

Here, in test_rosbag, the messages that corresponds to G in the example
above, is MessageMixed.
Try to create rule from BAG to CURRENT msg
045b7b1 
fixes CORE-6641

Consider the following scenario for a message G with the following
versions:

G0 -- rule.bmr --> G1 -- auto-generated rule --> G3

where G3 is the CURRENT msg, and G2 is the BAG message that needs
migration.

We need to check for a migration rule like this first:

G2 --> G3

instead of the backwards ones that were generated before this bug fix:

G2 --> G1

Note that the 'lazy' check to valid the rules in the make_update_rule
method makes that rule pass initially, but later it fails because
there's no rule for the sub fields.
Check the sub rules are also valid
bb71941 
refs CORE-6641

Checking the sub rules are also valid we allow to check for different
positions in the chain for the BAG message we need to migrate, because
we don't really know where it should go, and the 'lazy' check for valid
in the make_update_rule method makes the suggested rules pass almost
always, so we can easily get a rule in the wrong place that later would
fail because there are no rules for the sub fields.
@efernandez
Copy link
Contributor Author

Now that #1009 is merged, I've just rebased both #1010 and #1011 on top of lunar-devel, so they can be reviewed.

I recommend reviewing the test on #1010 and here only the two latest commits, which only bring a fairly minimal change compared to the test.
Remember that #1010 is up only for reviewing and showing the current logic fails with the new test. We should close that PR after the review and merge only #1011.

Thanks!

@dirk-thomas
Copy link
Member

@efernandez This looks good to me. Does it make sense to squash the last two commits?

@mikepurvis What do you think? Did you run this code as part of your testing?

@efernandez
Copy link
Contributor Author

efernandez commented Mar 20, 2017 •
edited
Loading

@dirk-thomas I think they cover two different things:

  1. 045b7b1 is the main fix for the new test case. It checks for a path directly to the last message first.
  2. bb71941 adds a premature check for sub-rules, so it allows to check more options if the sub-rules fail. Before, the first option with the top rules valid was picked, regardless of the sub-rules. With this commit, the premature check allows to discard options that would fail due to the sub-rules, and therefore check other options. This is an improvement in the line of this comment: https://github.com/clearpathrobotics/ros_comm/blob/bb71941a808dea0a7a9f11960818a00b4a8630db/tools/rosbag/src/rosbag/migration.py#L307

Regarding testing, @mikepurvis cherry-picked the last two commits (not the test stuff because it wasn't implemented by that time) and get it into the bundles we run on our robots https://github.com/ros/ros_comm/commits/lunar-edge (that has this commit 4c4358f, with the two fix commits from here). That's been running now for approximately 1 month with a message that needs the migration fix from this PR. Migration is done on the fly using the rosbag migration (Python) API.

@dirk-thomas
Copy link
Member

@efernandez Thanks for the explanation. Sounds good to me.

@dirk-thomas dirk-thomas merged commit af577c0 into ros:lunar-devel Mar 20, 2017
@dirk-thomas
Copy link
Member

Would you like to see these changes (including the ones from the previous PR) backported (and if yes, wo which ROS distros) or do you not mind since you use a fork anyway?

@mikepurvis
Copy link
Member

We are running from lunar-devel/lunar-edge now, so no need for backport from our side.

@efernandez efernandez deleted the fix_migration_rule branch March 20, 2017 20:32
@efernandez
Copy link
Contributor Author

Exactly, no need to backport.

And regarding public ROS packages with changed messages, there are only two AFAIK. One of them is this one:

trajectory_msgs$ l rules/
JointTrajectoryPoint_Groovy2Hydro_08.10.2013.bmr

@dirk-thomas dirk-thomas mentioned this pull request Oct 26, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
hitlist
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@efernandez @dirk-thomas @mikepurvis